GCC: unused warnings #8359

jeffro256 · 2022-05-28T18:16:15Z

This PR fixes 4 instances of -Wunused...

selsta · 2022-05-29T21:44:32Z

src/wallet/wallet2.cpp

@@ -5127,7 +5124,6 @@ std::string wallet2::exchange_multisig_keys(const epee::wipeable_string &passwor
    expanded_msgs.emplace_back(msg);

  // reconstruct multisig account
-  crypto::public_key dummy;


This one is already fixed in one of the multisig PRs. Can you undo this change?

mj-xmr · 2022-05-31T13:39:45Z

src/wallet/wallet2.cpp

@@ -3476,7 +3475,6 @@ void wallet2::refresh(bool trusted_daemon, uint64_t start_height, uint64_t & blo
      if (!first && blocks.empty())
      {
        m_node_rpc_proxy.set_height(m_blockchain.size());
-        refreshed = true;


I think this was set by the author to facilitate easier debugging, since with this variable in place, it appears in the locals debugging window.

I personally don't need it, but just saying that this is the reason why I haven't removed it myself.

That was my thought as well. I don't see any need to remove it.

It's a leftover from this patch: #6097 (comment)

@hyc pushed changes to keep the flag in and remove the warning

@jeffro256 "Easier debugging" was speculation. As it says in mooo's comment, he wanted to clean it up but probably forgot about it. We can ask him again to be sure but just silencing doesn't make much sense here.

Pinging @moneromooo-monero

12:34 <moneromooo> I had to reorder some code to fix... a timing info leak IIRC. In turn, this undid something I had fixed before, which I believe was a subtle race condition with the txpool. 12:35 <moneromooo> It was pretty subtle IIRC, and so I needed time to think about how to refix it after the move, and I never got to it. I do not remember the details of the race. 12:35 <moneromooo> I don't think I'm likely to ever get to it now. 12:43 <+selsta> ok ty, i guess i'll let the other decide if we keep the unused variable 12:45 <moneromooo> At this point, since it was to remind me, it could be replaced by a comment if people really hate it.

So either we remove the unused variable and add a comment or we keep it. Silencing doesn't make sense in my opinion.

+1 for commenting out rather than silencing.

There will surely be a lot of debugging to do there in future, but then it can be uncommented on demand, still not causing warnings to everybody else and not having to resort to compiler hacks, which might not be cross-platform.

Probably should add some of mooo's commentary as a comment in the source too. Saves digging thru github issues to find out what it's all about.

mj-xmr

It looks good, but please gather 2nd or even 3rd opinion about the wallet2 change.

mj-xmr · 2022-05-31T13:54:44Z

src/crypto/hash-extra-jh.c

@@ -38,5 +38,6 @@

 void hash_extra_jh(const void *data, size_t length, char *hash) {
  int r = jh_hash(HASH_SIZE * 8, data, 8 * length, (uint8_t*)hash);
+  (void) r; // silence -Wunused-variable


Before anybody mentions inconsistency of the casting style:

This is C, not C++, so static_cast<> can't be used.

So these are only unused because assert() is being no-op'd. These hash functions don't allocate memory so have no reason to fail. Maybe we can just get rid of r and the asserts too?

Yeah since hashbitlen is a compile-time constant, there should be no reason for jh_hash to not return SUCCESS

Okay changes are pushed for that

Hi, sorry for the wishy-washiness but I'm gonna take back what I said and go with your initial patch. Safer to leave the asserts in place so that they're present in Debug builds, and will catch any accidental misuses of the code in the future.

Should I make the assert statements conditional on whether NDEBUG is defined?

assert() is already a #define that is empty when NDEBUG is defined. Do you mean conditionalizing the other statements?

Yeah, if NDEBUG is defined, then don't check for error, otherwise do

I mean, there's really no point checking for an non-success status since the only possible failure condition is if hashbitlen is wrong. Since it's a compile-time constant in our case the only way someone could use it wrong is if they modified the hash-extra function bodies. I put a comment above that line in this PR which explains why we are not checking for a failure result.

OK, sounds good.

selsta · 2022-06-07T19:05:46Z

src/wallet/wallet2.cpp

+  // moneromooo-monero says this about the "refreshed" variable:
+  // "I had to reorder some code to fix... a timing info leak IIRC. In turn, this undid something I had fixed before, ... a subtle race condition with the txpool.
+  // It was pretty subtle IIRC, and so I needed time to think about how to refix it after the move, and I never got to it."


Suggested change

// moneromooo-monero says this about the "refreshed" variable:

// "I had to reorder some code to fix... a timing info leak IIRC. In turn, this undid something I had fixed before, ... a subtle race condition with the txpool.

// It was pretty subtle IIRC, and so I needed time to think about how to refix it after the move, and I never got to it."

// TODO: moneromooo-monero says this about the "refreshed" variable:

// "I had to reorder some code to fix... a timing info leak IIRC. In turn, this undid something I had fixed before, ... a subtle race condition with the txpool.

// It was pretty subtle IIRC, and so I needed time to think about how to refix it after the move, and I never got to it."

// https://github.com/monero-project/monero/pull/6097

TODO so that it can be quickly found via git grep. Also adding a URL for context.

moneromooo-monero · 2022-06-07T19:18:23Z

At this point I'm unlikely to spend a lot of time thinking about it and fixing it well. So if people want to kill the warning, be my guest. I'd be better if people wanted to fix it properly instead of just killing the warning, but that's up to them.

selsta · 2022-07-06T02:31:27Z

@jeffro256 Can you keep the refreshed variable untouched and only add the comment? We can keep the warning as a reminder for someone to look into, if it stays unfixed for another year then we can decide what to do next with it.

Sorry for this huge discussion :)

hash_extra: don't test for success in `jh_hash` and `skein_hash` since its guaranteed device_ledger: move anonymous global variable apdu_verbose into .cpp file Add comments to `refreshed` method variable in wallet2

jeffro256 · 2022-07-12T01:34:01Z

@selsta done!

selsta reviewed May 29, 2022

View reviewed changes

jeffro256 force-pushed the unused_warnings branch 2 times, most recently from b1710f6 to 50a9879 Compare May 29, 2022 21:57

mj-xmr reviewed May 31, 2022

View reviewed changes

mj-xmr approved these changes May 31, 2022

View reviewed changes

mj-xmr reviewed May 31, 2022

View reviewed changes

mj-xmr approved these changes May 31, 2022

View reviewed changes

jeffro256 force-pushed the unused_warnings branch 2 times, most recently from cc85eb4 to 82207fd Compare June 1, 2022 02:25

jeffro256 changed the title ~~GCC: general unused warnings~~ GCC: unused warnings Jun 1, 2022

selsta mentioned this pull request Jun 2, 2022

Wallet: Fix unused variable compiler warning #6310

Closed

jeffro256 force-pushed the unused_warnings branch from 82207fd to faa1be6 Compare June 3, 2022 11:48

selsta reviewed Jun 7, 2022

View reviewed changes

jeffro256 force-pushed the unused_warnings branch from faa1be6 to 1fc5de3 Compare June 7, 2022 19:17

GCC: fix some unused warnings

4d7f6f5

hash_extra: don't test for success in `jh_hash` and `skein_hash` since its guaranteed device_ledger: move anonymous global variable apdu_verbose into .cpp file Add comments to `refreshed` method variable in wallet2

jeffro256 force-pushed the unused_warnings branch from 1fc5de3 to 4d7f6f5 Compare July 12, 2022 01:32

mj-xmr approved these changes Jul 14, 2022

View reviewed changes

luigi1111 merged commit df02b56 into monero-project:master Aug 23, 2022

jeffro256 mentioned this pull request Dec 11, 2022

removed definition of refreshed bool #8673

Closed

jeffro256 deleted the unused_warnings branch October 31, 2023 07:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GCC: unused warnings #8359

GCC: unused warnings #8359

jeffro256 commented May 28, 2022 •

edited

Loading

selsta May 29, 2022

jeffro256 May 29, 2022

mj-xmr May 31, 2022 •

edited

Loading

hyc May 31, 2022

selsta May 31, 2022

jeffro256 Jun 1, 2022

selsta Jun 1, 2022

jeffro256 Jun 1, 2022

selsta Jun 2, 2022 •

edited

Loading

mj-xmr Jun 2, 2022

hyc Jun 2, 2022

jeffro256 Jun 7, 2022

mj-xmr left a comment •

edited

Loading

mj-xmr May 31, 2022 •

edited

Loading

hyc May 31, 2022

jeffro256 Jun 1, 2022

jeffro256 Jun 1, 2022

hyc Jun 1, 2022

jeffro256 Jun 1, 2022

hyc Jun 1, 2022

jeffro256 Jun 1, 2022

jeffro256 Jun 1, 2022

hyc Jun 1, 2022

selsta Jun 7, 2022

jeffro256 Jun 7, 2022

moneromooo-monero commented Jun 7, 2022

selsta commented Jul 6, 2022

jeffro256 commented Jul 12, 2022

GCC: unused warnings #8359

GCC: unused warnings #8359

Conversation

jeffro256 commented May 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mj-xmr May 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

selsta Jun 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mj-xmr left a comment • edited Loading

Choose a reason for hiding this comment

mj-xmr May 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moneromooo-monero commented Jun 7, 2022

selsta commented Jul 6, 2022

jeffro256 commented Jul 12, 2022

jeffro256 commented May 28, 2022 •

edited

Loading

mj-xmr May 31, 2022 •

edited

Loading

selsta Jun 2, 2022 •

edited

Loading

mj-xmr left a comment •

edited

Loading

mj-xmr May 31, 2022 •

edited

Loading